Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DiagnosticsToolbox #1228

Merged
merged 57 commits into from
Aug 28, 2023
Merged

DiagnosticsToolbox #1228

merged 57 commits into from
Aug 28, 2023

Conversation

andrewlee94
Copy link
Member

@andrewlee94 andrewlee94 commented Jul 25, 2023 •
edited
Loading

Fixes None

Summary/Motivation:

This PR adds the first iteration of the new DiagnosticsToolbox class. Documentation and tests are coming, but we need input and feedback on the API before it gets set in stone.

A draft tutorial can be found here: https://app.box.com/folder/218647852677 (for those with access to the IDAES Box folder).

Changes proposed in this PR:

  • New DiagnosticsToolbox class

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@andrewlee94 andrewlee94 self-assigned this Jul 25, 2023
@andrewlee94 andrewlee94 added enhancement New feature or request help wanted Extra attention is needed Priority:High High Priority Issue or PR core Issues dealing with core modeling components WIP diagnostics labels Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023 •
edited
Loading

Codecov Report

Patch coverage: 86.26% and project coverage change: +0.11% 🎉

Comparison is base (2e41a88) 76.59% compared to head (deeb154) 76.70%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1228      +/-   ##
==========================================
+ Coverage   76.59%   76.70%   +0.11%     
==========================================
  Files         382      382              
  Lines       60822    61182     +360     
  Branches    11200    11289      +89     
==========================================
+ Hits        46587    46932     +345     
  Misses      11818    11818              
- Partials     2417     2432      +15
Files Changed Coverage Δ
idaes/core/util/model_diagnostics.py 71.85% <85.38%> (+22.32%) ⬆️
idaes/core/util/__init__.py 100.00% <100.00%> (ø)
idaes/core/util/model_statistics.py 97.38% <100.00%> (+0.67%) ⬆️
idaes/core/util/scaling.py 90.47% <100.00%> (+1.99%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Robbybp
Robbybp reviewed Aug 8, 2023
View reviewed changes
Copy link
Member

@Robbybp Robbybp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks really good. Several minor comments, then I'd recommend printing out the independent diagonal blocks of the overconstrained/underconstrained subsystems individually, as this is often much easier to read if these sets decompose.

idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
@dallan-keylogic
Copy link
Contributor

I want to register a complaint that list_badly_scaled_variables is pretty feeble. This is what I got with it:

====================================================================================
The following variable(s) are poorly scaled:

    fs.condenser_flash.split.Liq_state[0.0].log_mole_frac_phase_comp[Liq,H2O]: 0.0009862360871915779

while this is what I got with extreme_jacobian_columns:

    2.00e-03, [fs.condenser_flash.control_volume.properties_in[0.0].log_mole_frac_comp[N2]]
    2.00e-03, [fs.condenser_flash.control_volume.properties_in[0.0].log_mole_frac_phase_comp[Vap,N2]]
    2.00e-03, [fs.condenser_flash.control_volume.properties_out[0.0].log_mole_frac_comp[N2]]
    2.00e-03, [fs.condenser_flash.control_volume.properties_out[0.0].log_mole_frac_phase_comp[Vap,N2]]
    5.29e-05, [fs.condenser_flash.split.Vap_state[0.0].temperature_dew[Liq,Vap]]
    2.00e-03, [fs.condenser_flash.split.Vap_state[0.0].log_mole_frac_comp[N2]]
    2.08e-03, [fs.condenser_flash.split.Vap_state[0.0].log_mole_frac_phase_comp[Vap,N2]]
    1.33e-13, [fs.condenser_flash.split.Liq_state[0.0].phase_frac[Liq]]
    1.33e-13, [fs.condenser_flash.split.Liq_state[0.0].phase_frac[Vap]]

I suppose 2e-3 is above the cutoff for the default setting, so that isn't an issue. It did miss the issue with the dew point and the enormous, matrix-is-singular issue with the phase fractions.

Also you need to make clear to the user that, whatever these methods return, they aren't going to be inerrant signs a variable or constraint is badly scaled, but just a sign you need to look at that variable or constraint to ensure it is well-scaled.

@andrewlee94
Copy link
Member Author

@dallan-keylogic The poorly scaled variables check currently uses the badly_scaled_variables method out of the scaling toolbox, so it is a rather simple check. We used this for the prototype in order to have a basic demonstration using existing tools that were robust - I hope to either supplement or replace this with an svd tool once we have time to address the current issues and limitation with that tool.

@dallan-keylogic
Copy link
Contributor

@andrewlee94 , You don't need to do a full SVD in order to use the extreme columns/rows tools, you just need a representation of the Jacobian. Creating a Jacobian from the block takes a bit of time, but not nearly as much as doing a full SVD.

Robbybp
Robbybp approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@Robbybp Robbybp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the docstring formatting, I think this is ready.

@andrewlee94 andrewlee94 mentioned this pull request Aug 25, 2023
Merged
@andrewlee94 andrewlee94 removed the WIP label Aug 25, 2023
blnicho
blnicho reviewed Aug 28, 2023
View reviewed changes
docs/explanations/model_diagnostics/index.rst Outdated Show resolved Hide resolved
idaes/core/util/model_statistics.py Outdated Show resolved Hide resolved
idaes/core/util/model_statistics.py Outdated Show resolved Hide resolved
idaes/core/util/tests/test_model_statistics.py Outdated Show resolved Hide resolved
idaes/core/util/tests/test_model_statistics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
idaes/core/util/model_diagnostics.py Outdated Show resolved Hide resolved
@andrewlee94 andrewlee94 enabled auto-merge (squash) August 28, 2023 23:15
@andrewlee94 andrewlee94 merged commit b01bc20 into IDAES:main Aug 28, 2023
40 checks passed
@andrewlee94 andrewlee94 mentioned this pull request Sep 7, 2023
Open
31 tasks
@andrewlee94 andrewlee94 deleted the diagnostics_1 branch February 22, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Robbybp Robbybp Robbybp approved these changes

@adam-a-a adam-a-a adam-a-a left review comments

@lbianchi-lbl lbianchi-lbl lbianchi-lbl left review comments

@blnicho blnicho blnicho approved these changes

@carldlaird carldlaird Awaiting requested review from carldlaird

@bpaul4 bpaul4 Awaiting requested review from bpaul4 bpaul4 is a code owner

@eslickj eslickj Awaiting requested review from eslickj

@dallan-keylogic dallan-keylogic Awaiting requested review from dallan-keylogic dallan-keylogic is a code owner

@ksbeattie ksbeattie Awaiting requested review from ksbeattie

@jsiirola jsiirola Awaiting requested review from jsiirola

@adowling2 adowling2 Awaiting requested review from adowling2

Assignees

@andrewlee94 andrewlee94

Labels
core Issues dealing with core modeling components diagnostics enhancement New feature or request help wanted Extra attention is needed Priority:High High Priority Issue or PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@andrewlee94 @dallan-keylogic @Robbybp @blnicho @adam-a-a @lbianchi-lbl